Skip to content

Conversation

Enna1
Copy link
Contributor

@Enna1 Enna1 commented Aug 14, 2025

In #100281, we use TaggedBlocks to record blocks modified by CheckIfPHIMatche(), so do not need to clear every block in BlockList if CheckIfPHIMatches() match failed.

If CheckIfPHIMatches() match succeed, we can reuse TaggedBlocks to only record matching PHIs for modified blocks, avoid checking every block in BlockList to see if PHITag is set.

…ordMatchingPHIs

In #100281, we use `TaggedBlocks`
to record blocks modified by `CheckIfPHIMatche()`, so do not need to clear
every block in `BlockList` if `CheckIfPHIMatches()` match failed.

If `CheckIfPHIMatches()` match succeed, we can reuse `TaggedBlocks` to
only record matching PHIs for modified blocks, avoid checking every block in
`BlockList` to see if PHITag is set.
@llvmbot
Copy link
Member

llvmbot commented Aug 14, 2025

@llvm/pr-subscribers-llvm-transforms

Author: Mingjie Xu (Enna1)

Changes

In #100281, we use TaggedBlocks to record blocks modified by CheckIfPHIMatche(), so do not need to clear every block in BlockList if CheckIfPHIMatches() match failed.

If CheckIfPHIMatches() match succeed, we can reuse TaggedBlocks to only record matching PHIs for modified blocks, avoid checking every block in BlockList to see if PHITag is set.


Full diff: https://github.com/llvm/llvm-project/pull/153596.diff

1 Files Affected:

  • (modified) llvm/include/llvm/Transforms/Utils/SSAUpdaterImpl.h (+13-13)
diff --git a/llvm/include/llvm/Transforms/Utils/SSAUpdaterImpl.h b/llvm/include/llvm/Transforms/Utils/SSAUpdaterImpl.h
index 746926e5bee33..52fe3a6f4baf4 100644
--- a/llvm/include/llvm/Transforms/Utils/SSAUpdaterImpl.h
+++ b/llvm/include/llvm/Transforms/Utils/SSAUpdaterImpl.h
@@ -366,7 +366,7 @@ class SSAUpdaterImpl {
         continue;
 
       // Look for an existing PHI.
-      FindExistingPHI(Info->BB, BlockList);
+      FindExistingPHI(Info->BB);
       if (Info->AvailableVal)
         continue;
 
@@ -412,11 +412,11 @@ class SSAUpdaterImpl {
 
   /// FindExistingPHI - Look through the PHI nodes in a block to see if any of
   /// them match what is needed.
-  void FindExistingPHI(BlkT *BB, BlockListTy *BlockList) {
+  void FindExistingPHI(BlkT *BB) {
     SmallVector<BBInfo *, 20> TaggedBlocks;
     for (auto &SomePHI : BB->phis()) {
       if (CheckIfPHIMatches(&SomePHI, TaggedBlocks)) {
-        RecordMatchingPHIs(BlockList);
+        RecordMatchingPHIs(TaggedBlocks);
         break;
       }
     }
@@ -424,7 +424,7 @@ class SSAUpdaterImpl {
 
   /// CheckIfPHIMatches - Check if a PHI node matches the placement and values
   /// in the BBMap.
-  bool CheckIfPHIMatches(PhiT *PHI, SmallVectorImpl<BBInfo *> &TaggedBlocks) {
+  bool CheckIfPHIMatches(PhiT *PHI, BlockListTy &TaggedBlocks) {
     // Match failed: clear all the PHITag values. Only need to clear visited
     // blocks.
     auto Cleanup = make_scope_exit([&]() {
@@ -484,15 +484,15 @@ class SSAUpdaterImpl {
 
   /// RecordMatchingPHIs - For each PHI node that matches, record it in both
   /// the BBMap and the AvailableVals mapping.
-  void RecordMatchingPHIs(BlockListTy *BlockList) {
-    for (typename BlockListTy::iterator I = BlockList->begin(),
-           E = BlockList->end(); I != E; ++I)
-      if (PhiT *PHI = (*I)->PHITag) {
-        BlkT *BB = PHI->getParent();
-        ValT PHIVal = Traits::GetPHIValue(PHI);
-        (*AvailableVals)[BB] = PHIVal;
-        BBMap[BB]->AvailableVal = PHIVal;
-      }
+  void RecordMatchingPHIs(BlockListTy &TaggedBlocks) {
+    for (BBInfo *Block : TaggedBlocks) {
+      PhiT *PHI = Block->PHITag;
+      assert(PHI && "PHITag didn't set?");
+      BlkT *BB = PHI->getParent();
+      ValT PHIVal = Traits::GetPHIValue(PHI);
+      (*AvailableVals)[BB] = PHIVal;
+      BBMap[BB]->AvailableVal = PHIVal;
+    }
   }
 };
 

@nikic nikic changed the title [SSAUpdater] Only iterate blocks modified by CheckIfPHIMatches() in ordMatchingPHIs() [SSAUpdater] Only iterate blocks modified by CheckIfPHIMatches() in RecordMatchingPHIs() Aug 14, 2025
Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Enna1 Enna1 merged commit a293573 into main Aug 16, 2025
11 checks passed
@Enna1 Enna1 deleted the users/Enna1/NFC-SSAUpdater-RecordMatchingPHIs branch August 16, 2025 11:59
@llvm-ci
Copy link
Collaborator

llvm-ci commented Aug 16, 2025

LLVM Buildbot has detected a new failure on builder openmp-s390x-linux running on systemz-1 while building llvm at step 6 "test-openmp".

Full details are available at: https://lab.llvm.org/buildbot/#/builders/88/builds/15136

Here is the relevant piece of the build log for the reference
Step 6 (test-openmp) failure: test (failure)
******************** TEST 'libomp :: tasking/issue-94260-2.c' FAILED ********************
Exit Code: -11

Command Output (stdout):
--
# RUN: at line 1
/home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/./bin/clang -fopenmp   -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test -L /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/src  -fno-omit-frame-pointer -mbackchain -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test/ompt /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test/tasking/issue-94260-2.c -o /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp -lm -latomic && /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp
# executed command: /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/./bin/clang -fopenmp -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test -L /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/src -fno-omit-frame-pointer -mbackchain -I /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test/ompt /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.src/openmp/runtime/test/tasking/issue-94260-2.c -o /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp -lm -latomic
# executed command: /home/uweigand/sandbox/buildbot/openmp-s390x-linux/llvm.build/runtimes/runtimes-bins/openmp/runtime/test/tasking/Output/issue-94260-2.c.tmp
# note: command had no output on stdout or stderr
# error: command failed with exit status: -11

--

********************


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants